-
Notifications
You must be signed in to change notification settings - Fork 518
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Engineering Design Interface (EDI) Contrib Package #2937
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First batch of comments on the documentation (lots of duplicate comments across different files)
Automated unpacking for multi-use black-boxes | ||
--------------------------------------------- | ||
|
||
Coding a black-box model often represents a significant effort, and it is therefore desirable to have the black-box work in many situations. A common case is to have a black-box model with a scalar input variable perform vectorized operations, ie, take in a vector and return a vector. In the more general case, this can be thought of as passing in multiple run-cases as input to the black-box model. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Coding a black-box model often represents a significant effort, and it is therefore desirable to have the black-box work in many situations. A common case is to have a black-box model with a scalar input variable perform vectorized operations, ie, take in a vector and return a vector. In the more general case, this can be thought of as passing in multiple run-cases as input to the black-box model. | |
Coding a black-box model often represents a significant effort, and it is therefore desirable to have the black-box work in many situations. A common case is to have a black-box model with a scalar input variable perform vectorized operations, i.e., take in a vector and return a vector. In the more general case, this can be thought of as passing in multiple run-cases as input to the black-box model. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file has been removed from the documentation
z *= units.ft**2 | ||
dzdx *= units.ft # units.ft**2 / units.ft | ||
dzdy *= units.ft # units.ft**2 / units.ft |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you hard-code the units here instead of reading it from self.outputs['z']
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't think of it. Fixed.
f.ConstraintList([[z, '==', [x, y], UnitCircle()], z <= 1 * units.m**2]) | ||
|
||
# ============================================= | ||
# Run the black box (improves coverage metrics) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what is meant by "improves coverage metrics". Originally I thought this might be EDI-specific terminology but I didn't see it mentioned anywhere else in the documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No idea what I meant by this, removed. I think I may have been referring to the order of things in the file it may have just been copy paste vestige.
.. py:function:: f.Constraint(expr) | ||
|
||
Declares a constraint in a pyomo.edi.formulation | ||
|
||
:param expr: The expression representing the constraint | ||
:type expr: pyomo expression | ||
|
||
:return: None | ||
:rtype: None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd recommend including this as a docstring in the code and using the automethod Sphinx command instead of writing this directly in the documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed above
.. py:function:: f.ConstraintList(conList) | ||
|
||
Declares new constraints in a pyomo.edi.formulation from a list of inputs | ||
|
||
:param conList: The list of constraints to be generated. Entries will be pyomo expressions, or lists/tuples/dicts that are used to create RuntimeConstraints (see :doc:`here <./blackboxconstraints>`) | ||
:type conList: list | ||
|
||
:return: None | ||
:rtype: None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd recommend including this as a docstring in the code and using the automethod Sphinx command instead of writing this directly in the documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed above
|
||
.. py:function:: f.Constraint(expr) | ||
|
||
Declares a constraint in a pyomo.edi.formulation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: pyomo.contrib.edi.formulation
(appears multiple times in this file)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Next batch of comments. I'm still going through the code.
Relation to Pyomo Constraint | ||
---------------------------- | ||
|
||
The EDI constraint constructor is essentially a direct pass through to base pyomo. Constraints will be added to the ``pyomo.ConcreteModel`` in increasing order with key ``constraint_###`` where the the index of the objective appears after the underscore. First constraint is labeled as ``constraint_1``, and constraint names are never padded with zeros. RuntimeConstraints also contribute to this counter. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The EDI constraint constructor is essentially a direct pass through to base pyomo. Constraints will be added to the ``pyomo.ConcreteModel`` in increasing order with key ``constraint_###`` where the the index of the objective appears after the underscore. First constraint is labeled as ``constraint_1``, and constraint names are never padded with zeros. RuntimeConstraints also contribute to this counter. | |
The EDI constraint constructor is essentially a direct pass through to base Pyomo. Constraints will be added to the ``pyomo.ConcreteModel`` in increasing order with key ``constraint_###`` where the the index of the objective appears after the underscore. First constraint is labeled as ``constraint_1``, and constraint names are never padded with zeros. RuntimeConstraints also contribute to this counter. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
adopted
Known Issues | ||
------------ | ||
|
||
* Indexed variables must be broken up using either indices or a pyomo rule (see `this issue <https://github.com/codykarcher/pyomo/issues/3>`__) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* Indexed variables must be broken up using either indices or a pyomo rule (see `this issue <https://github.com/codykarcher/pyomo/issues/3>`__) | |
* Indexed variables must be broken up using either indices or a Pyomo rule (see `this issue <https://github.com/codykarcher/pyomo/issues/3>`__) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
|
||
While some constraints are explicitly known and can be written directly into the optimization problem, it is common (particularly in engineering design) for some relationships to be too complex to be directly coded as a constraint. | ||
|
||
EDI refers to these types of constraints as ``RuntimeConstraints`` because they are not constructed until they are needed by the solver. A particular subset of Runtime Constraints of interest are Black-Box constraints, that is, constraints which call to an external routine. To the average pyomo and EDI user, ``RuntimeConstraints`` are for all intents and purposes Black-Box constraint, and the distinction is semantic. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EDI refers to these types of constraints as ``RuntimeConstraints`` because they are not constructed until they are needed by the solver. A particular subset of Runtime Constraints of interest are Black-Box constraints, that is, constraints which call to an external routine. To the average pyomo and EDI user, ``RuntimeConstraints`` are for all intents and purposes Black-Box constraint, and the distinction is semantic. | |
EDI refers to these types of constraints as ``RuntimeConstraints`` because they are not constructed until they are needed by the solver. A particular subset of Runtime Constraints of interest are Black-Box constraints, that is, constraints which call to an external routine. To the average Pyomo and EDI user, ``RuntimeConstraints`` are for all intents and purposes Black-Box constraints, and the distinction is semantic. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
|
||
In other words, if you wish to code a black-box constraint using EDI, you will be using the Runtime Constraint constructor. | ||
|
||
In this context, a *Black-Box* is defined as a routine that performs hidden computation not visible EDI, pyomo, or more generally the optimization algorithm. However, it is **not** assumed that black-boxes are unable to return gradient information. A black-box in this context may be capable of returning arbitrary derivative information. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this context, a *Black-Box* is defined as a routine that performs hidden computation not visible EDI, pyomo, or more generally the optimization algorithm. However, it is **not** assumed that black-boxes are unable to return gradient information. A black-box in this context may be capable of returning arbitrary derivative information. | |
In this context, a *Black-Box* is defined as a routine that performs hidden computation not visible EDI, Pyomo, or more generally the optimization algorithm. However, it is **not** assumed that black-boxes are unable to return gradient information. A black-box in this context may be capable of returning arbitrary derivative information. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
Constructing a Black Box | ||
++++++++++++++++++++++++ | ||
|
||
First, we need to create an object which is visible to pyomo/EDI that calls the black-box function. EDI calls this a ``BlackBoxFunctionModel``, and it is a base class that gets inherited into the objects you will create as a user. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First, we need to create an object which is visible to pyomo/EDI that calls the black-box function. EDI calls this a ``BlackBoxFunctionModel``, and it is a base class that gets inherited into the objects you will create as a user. | |
First, we need to create an object which is visible to Pyomo/EDI that calls the black-box function. EDI calls this a ``BlackBoxFunctionModel``, and it is a base class that gets inherited into the objects you will create as a user. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
Check outputs | ||
------------- | ||
|
||
There is a ``checkOutputs()`` method that is not supported in the current version. Contact the developers if you desire this functionality, but the following the practices described in this documentation should render the need for this moot. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a ``checkOutputs()`` method that is not supported in the current version. Contact the developers if you desire this functionality, but the following the practices described in this documentation should render the need for this moot. | |
There is a ``checkOutputs()`` method that is not supported in the current version. Contact the developers if you desire this functionality, but following the practices described in this documentation should render the need for this moot. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file has been removed from the doc tree
import pyomo.environ as pyo | ||
from pyomo.environ import units | ||
from pyomo.contrib.edi import Formulation | ||
from pyomo.contrib.edi import BlackBoxFunctionModel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why import this if it isn't used in this example?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because I'm dumb. Fixed
Developers | ||
---------- | ||
|
||
The pyomo EDI interface is developed and maintained by `Cody Karcher <https://github.com/codykarcher>`_ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pyomo EDI interface is developed and maintained by `Cody Karcher <https://github.com/codykarcher>`_ | |
The Pyomo EDI interface is developed and maintained by `Cody Karcher <https://github.com/codykarcher>`_ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
pyomo/contrib/edi/__init__.py
Outdated
from pyomo.contrib.edi.blackBoxFunctionModel import ( | ||
BlackBoxFunctionModel_Variable as BlackBoxVariable, | ||
) | ||
from pyomo.contrib.edi.blackBoxFunctionModel import ( | ||
BlackBoxFunctionModel_Variable as BBVariable, | ||
) | ||
from pyomo.contrib.edi.blackBoxFunctionModel import ( | ||
BlackBoxFunctionModel_Variable as BBV, | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you importing the same class multiple times with different names? I'm guessing it's an effort to provide shorter name aliases to users but I don't think this is a good idea. It obfuscates the real class name and will cause confusion over the differences between them. If the user doesn't want to type the full class name then the user can do their own class aliasing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was convenient for me, but point taken. Have removed the naming alternatives.
) | ||
|
||
|
||
class BlackBoxFunctionModel_Variable(object): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This class isn't described in the documentation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a user-facing class and therefore was not prioritized for the documentation. It can be added in future developer documentation if desired
Fixes # N/A
Summary/Motivation:
This contrib is a light wrapper on the pyomo API that allows for alternative problem construction
Changes proposed in this PR:
Legal Acknowledgement
By contributing to this software project, I have read the contribution guide and agree to the following terms and conditions for my contribution: